[IBC] Implement the ICS-02 Client interfaces #933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
h5law
changed the base branch from
ibc/event_height
to
ibc/client_semantics_interfaces
|
References for the reviewer: CC: @Olshansk |
|
@srdtrk if you are able to review this PR when you have some time I would really appreciate your feedback. This is an implementation of ICS-02, specifically for enabling ICS-08 (not implemented here). The code should look and feel similar to |
Olshansk
requested changes
Aug 2, 2023
There was a problem hiding this comment.
Did my best to read, understand and review this but I'm sure I missed a bunch of stuff along the way.
That aside, great job! Well structured, easy to follow, some of the most understandable code I've seen, especially given the level of complexity and number of moving parts!
Comment on lines
+42
to
+43
| // This function is used to validate the state of a client running on a | ||
| // counterparty chain. |
There was a problem hiding this comment.
This comment is confusing me.
Is the purpose of this to verify the state of pocket on another chain, or the state of another chain on pocket?
There was a problem hiding this comment.
So this function gets the current network state and is used in the verification of a pocket client running on another network. I will try to clarify the comment
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // TODO_AFTER(#705): use the actual MinimumBlockTime once set |
| type ClientMessage interface | ||
| ``` | ||
|
|
||
| These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client. |
There was a problem hiding this comment.
Suggested change
| These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client. | |
| These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field, which contains a serialised, opaque data structure for use within the WASM client. |
|
|
||
| These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client. | ||
|
|
||
| The `data` field is a JSON serialised payload that contains the data required for the client to carry out the desired operation, as well as the operation name to carry out. For example, a verify membership payload is constructed using the following `struct`s: |
There was a problem hiding this comment.
This is a good instance of embedding the structure in the code: it's an example.
| ) | ||
| ``` | ||
|
|
||
| By utilising this pattern of JSON payloads the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState` for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage. |
There was a problem hiding this comment.
Suggested change
| By utilising this pattern of JSON payloads the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState` for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage. | |
| By utilising this pattern of JSON payloads, the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState`, for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage. |
|
|
||
| ## Provable Stores | ||
|
|
||
| ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this the provable stores must be initialised on a per client ID basis. This means that any operation using the provable store does not require the use of the `clientID`. This is done as follows: |
There was a problem hiding this comment.
Suggested change
| ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this the provable stores must be initialised on a per client ID basis. This means that any operation using the provable store does not require the use of the `clientID`. This is done as follows: | |
| ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this, the provable stores must be initialised on a per-client ID basis. This means that any operation using the provable store does not require using the `clientID`. This is done as follows: |
Comment on lines
+126
to
+134
| 1. `ClientState` | ||
| - `ClientState` is an opaque data structure defined by a client type. It may keep arbitrary internal state to track verified roots and past misbehaviours. | ||
| 2. `ConsensusState` | ||
| - `ConsensusState` is an opaque data structure defined by a client type, used by the | ||
| validity predicate to verify new commits & state roots. Likely the structure will contain the last commit produced by the consensus process, including signatures and validator set metadata. | ||
| 3. `ClientMessage` | ||
| - `ClientMessage` is an opaque data structure defined by a client type which provides information to update the client. `ClientMessage`s can be submitted to an associated client to add new `ConsensusState`(s) and/or update the `ClientState`. They likely contain a height, a proof, a commitment root, and possibly updates to the validity predicate. | ||
| 4. `Height` | ||
| - `Height` is an interface that defines the methods required by a clients implementation of their own height object `Height`s usually have two components: revision number and revision height. |
There was a problem hiding this comment.
Maybe add these to the definitions seciton above?
It's good context as one starts reading everything.
Suggested change
| 1. `ClientState` | |
| - `ClientState` is an opaque data structure defined by a client type. It may keep arbitrary internal state to track verified roots and past misbehaviours. | |
| 2. `ConsensusState` | |
| - `ConsensusState` is an opaque data structure defined by a client type, used by the | |
| validity predicate to verify new commits & state roots. Likely the structure will contain the last commit produced by the consensus process, including signatures and validator set metadata. | |
| 3. `ClientMessage` | |
| - `ClientMessage` is an opaque data structure defined by a client type which provides information to update the client. `ClientMessage`s can be submitted to an associated client to add new `ConsensusState`(s) and/or update the `ClientState`. They likely contain a height, a proof, a commitment root, and possibly updates to the validity predicate. | |
| 4. `Height` | |
| - `Height` is an interface that defines the methods required by a clients implementation of their own height object `Height`s usually have two components: revision number and revision height. | |
| 1. `ClientState` - an opaque data structure defined by a client type. It may keep an arbitrary internal state to track verified roots and past misbehaviours. | |
| 2. `ConsensusState` - an opaque data structure defined by a client type, used by the validity predicate to verify new commits & state roots. Likely the structure will contain the last commit produced by the consensus process, including signatures and validator set metadata. | |
| 3. `ClientMessage` - an opaque data structure defined by a client type which provides information to update the client. `ClientMessage`s can be submitted to an associated client to add new `ConsensusState`(s) and/or update the `ClientState`. They likely contain a height, a proof, a commitment root, and possibly updates to the validity predicate. | |
| 4. `Height` - an interface that defines the methods required by a clients implementation of their own height object `Height`s usually have two components: revision number and revision height. |
|
@srdtrk Let us know if/when you can take a look at this as well! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Summary generated by Reviewpad on 26 Jul 23 10:09 UTC
This pull request introduces various changes across multiple files in the codebase.
Here is a summary of the changes:
ibc/main_test.gohas undergone changes related to the constructor method call from "newProvableStore" to "NewProvableStore".ibc/main_test.gohas changes in the functionnewTestP2PModule,bus.RegisterModule(p2pMock)line removal, andprepareEnvironmentfunction modification.queries.goin theibc/client/typespackage includes the addition of new functions for retrieving and storing client and consensus states.submodule.goin theibc/clientpackage adds the implementation of a client manager module for managing the creation, update, and upgrade of clients.ibc/client/types/validate_test.gocontains new test cases for validating various types in thetypespackage.keys_ics02.goin theibc/pathpackage includes changes related to store keys and paths for client and consensus states.provable_store_test.gohas changes in the test cases and the constructor method call, replacingnewProvableStorewithNewProvableStore.queries.goin theibc/clientpackage introduces new functions for retrieving client and consensus states.upgrade.goadds new types and functions related to upgrading the client state.events.goincludes implementations related to emitting events for the IBC client.event_keys.godefines constants and variables related to the events emitted by the Client submodule.misbehaviour.goadds a new struct and methods related to handling misbehavior by clients.header.goadds a new file with types and methods related to the Wasm client consensus algorithm.consensus_state.goincludes the implementation of the ConsensusState struct and its associated methods.ibc/docs/README.mdhas additions related to the ICS-02 implementation and links to relevant resources.fraction.gointroduces a new file with types and methods for comparing and performing operations on fractions.errors.goincludes changes related to error codes and a new error.update.goadds types and methods for verifying and updating client states.introspect.gointroduces new functions for retrieving and verifying client and consensus states.types.goadds implementations related to the IBC client for a Wasm module.ibc/host/submodule.goincludes changes related to imports and function modifications.Please review these changes carefully to ensure they adhere to the code's functionality and requirements.
Issue
Fixes #894
Type of change
Please mark the relevant option(s):
List of changes
ClientManagerinterface in the client submoduleTesting
make develop_test; if any code changes were mademake test_e2eon k8s LocalNet; if any code changes were madee2e-devnet-testpasses tests on DevNet; if any code was changedRequired Checklist
godocformat comments on touched members (see: tip.golang.org/doc/comment)If Applicable Checklist
shared/docs/*if I updatedshared/*README(s)